Skip to content

fix(gc): allow retained array-growth aliases during copying verification - #9822

Closed
proggeramlug wants to merge 2 commits into
PerryTS:mainfrom
proggeramlug:fix/4644-retained-growth-verifier
Closed

fix(gc): allow retained array-growth aliases during copying verification#9822
proggeramlug wants to merge 2 commits into
PerryTS:mainfrom
proggeramlug:fix/4644-retained-growth-verifier

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Solid's effect computation can retain its sources array's original address after that tenured array grows. Perry deliberately preserves the old address as an array-growth forwarding stub, but PERRY_GC_VERIFY_EVACUATION=1 rejected this valid old-to-old alias during a copying minor.

Carry the evacuation verifier's policy through heap fields, remembered ranges, stack/global roots, mutable runtime roots, metadata, and copy-only/FFI scanners. Copying verification permits a forwarding hop only when both ends are retained array allocations; it still follows the entire chain and rejects young targets, including indirect ones. The non-copying and old-page evacuation path keeps its existing strict policy. Mutating rewrites and array access are unchanged.

The PASS1_MARKED source pin is updated after reviewing the sole cycle.rs change: selecting the strict verifier policy remains in minor finalization, outside the synchronous full-cycle census window.

Validation:

  • The new old-object/array-growth regression fails on main with the same heap-field verifier panic, then passes with this fix.
  • Controls exercise retained chains in four root formats and copy-only/FFI roots, and reject direct/indirect from-space references, including an unforwarded young target.
  • Full runtime suite: 3,171 passed, four existing ignores (single-threaded).
  • Native Solid 1.9.15 audit: all 20 output lines match Node, with four native modules and zero JavaScript modules. The original seed 4644 at rate 1 passes with evacuation verification and protected from-space: 22 copying minors, 14,022 moved objects.
  • All 64 local lint/compile checks covered, including workspace Clippy and all-targets -D warnings; the two source-pin checks passed on rerun after the audit update. Two CI-only expressions are skipped by the local harness. Formatting and diff checks pass.

Follow-up to #9821; relates to #4644. This resolves the audit's verifier finding; the native UI bridge remains separate. No version bump.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed false evacuation-verifier aborts during copying minor garbage collection when retained array-growth references are present.
    • Improved forwarding-reference verification across chained references while continuing to reject invalid nursery references.
    • Preserved strict validation for full old-generation evacuation scenarios.
  • Tests

    • Added coverage for retained array-growth aliases, multi-step forwarding chains, and invalid from-space references across supported root types.

@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: becd980f-4790-4035-a9cc-4e3fec7061a3

📥 Commits

Reviewing files that changed from the base of the PR and between 3705978 and 350d1b4.

📒 Files selected for processing (3)
  • crates/perry-runtime/src/gc/copying.rs
  • crates/perry-runtime/src/gc/tests/mod.rs
  • scripts/gc_runtime_root_holders.json

Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

Changes

Retained growth verification

Layer / File(s) Summary
Verifier policy and forwarding traversal
crates/perry-runtime/src/gc/verify.rs
EvacuationVerifier now permits retained old-generation array-growth aliases during copying-minor verification while rejecting stale from-space references.
Verifier wiring and root scanning
crates/perry-runtime/src/gc/roots.rs, crates/perry-runtime/src/gc/copying.rs, crates/perry-runtime/src/gc/cycle.rs, crates/perry-runtime/src/gc/tests/runtime_roots/*
Verification paths now receive configured copying-minor or all-forwarded verifiers.
Minor collection diagnostics
crates/perry-runtime/src/gc/copying.rs
Optional survival diagnostics now record worklist, promotion, drain, and remembered-set information during minor collection.
Forwarding coverage and audit updates
crates/perry-runtime/src/gc/tests/forwarding_verification.rs, crates/perry-runtime/src/gc/tests/mod.rs, changelog.d/9822-retained-growth-verifier.md, scripts/gc_runtime_root_holders.json
Tests cover retained aliases, forwarding chains, root formats, heap slots, and from-space rejection. The changelog and audit metadata are updated.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to 350d1

This change permits valid retained array-growth aliases during copying minor GC while retaining rejection of stale from-space references and strict verification elsewhere. The supplied coverage indicates no remaining merge-blocking risk.

Sequence Diagram(s)

sequenceDiagram
  participant CopyingMinorGC
  participant EvacuationVerifier
  participant RuntimeRootVisitor
  participant HeapObjectVerifier
  CopyingMinorGC->>EvacuationVerifier: create copying_minor verifier
  CopyingMinorGC->>RuntimeRootVisitor: verify runtime roots
  RuntimeRootVisitor->>EvacuationVerifier: inspect forwarded references
  CopyingMinorGC->>HeapObjectVerifier: verify heap fields
  HeapObjectVerifier->>EvacuationVerifier: inspect forwarded references
  EvacuationVerifier-->>CopyingMinorGC: accept retained alias or report stale reference
Loading

Suggested reviewers: jdalton

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 47.37% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 8 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main change: allowing retained array-growth aliases during copying verification.
Description check ✅ Passed The description explains the problem, implementation scope, preserved behavior, related issues, and detailed validation results. It does not reproduce the template headings or checklist items, but it …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 47.37% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 8 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed on main via merge train #9866 (rebase-merged, so your commits keep their authorship). Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant